Skip to content

Conversation

@adamdickmeiss
Copy link

@adamdickmeiss adamdickmeiss commented Nov 10, 2025

@adamdickmeiss adamdickmeiss force-pushed the sql-connection-pool-in-use-metrics branch from 3d71a14 to b46ec6d Compare November 10, 2025 10:29
vietj
vietj previously requested changes Nov 10, 2025
Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you contribute a test for this ?

@adamdickmeiss adamdickmeiss force-pushed the sql-connection-pool-in-use-metrics branch from 7c43127 to b46ec6d Compare November 10, 2025 15:17
@adamdickmeiss adamdickmeiss requested a review from vietj November 10, 2025 15:53
Begin/End metrics size depends on the timeout parameter value.

When a timeout is expected, a single begin/end metric should be listed, corresponding to the getConnection/close calls wrapping the tasks.

When a timeout isn't expected, the number of begin/end metric is the number of tasks plus one, corresponding to the getConnection/close calls.

Signed-off-by: Thomas Segismont <[email protected]>
Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont tsegismont dismissed vietj’s stale review November 12, 2025 17:08

Test has been contributed

@tsegismont tsegismont removed the request for review from vietj November 12, 2025 17:08
@tsegismont tsegismont added this to the 5.0.6 milestone Nov 12, 2025
@tsegismont tsegismont changed the title Add in_use metrics for sql connection pool SqlConnectionPool does not invoke PoolMetrics begin/end methods Nov 12, 2025
Since metricsEnd can be invoked asynchronously, relax the test of usage size.

Signed-off-by: Thomas Segismont <[email protected]>
@tsegismont tsegismont merged commit cee8982 into eclipse-vertx:5.0 Nov 13, 2025
18 checks passed
@tsegismont
Copy link
Contributor

Thank you @adamdickmeiss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants